@git-hulk @ShooterIT Do you guys remember how this atomicity was broken? Is there any example?

seem the change casue this issue:

127.0.0.1:6379> flushall
OK
127.0.0.1:6379> msetnx k v1 k v2
(integer) 1
127.0.0.1:6379> get k
"v2

127.0.0.1:6666> flushall
OK
127.0.0.1:6666> msetnx k v1 k v2
(integer) 0
127.0.0.1:6666> get k
"v1"

Redis allow we overriding the same key but kvrocks will fail in this case

Hi @enjoy-binbin

Is there any example?

It should be a bit hard to reproduce, but we did exist since the MSET would write key-value one by one, so it will be partially written the DB if Kvrocks exited in the middle way. The inconsistent behavior you mentioned should be also affected by this implementation.

we already checked whether the key exists before:

  for (StringPair pair : pairs) {
    keys.emplace_back(pair.key);
  }
  if (Exists(keys, &exists).ok() && exists > 0) {
    return rocksdb::Status::OK();
  }

so the exist in for loop looks a bit redundant, in which case we need this exist (the for loop one, i.e. this PR diff)?
and another question is do we want to fix this inconsistency?

we already checked whether the key exists before:

  for (StringPair pair : pairs) {
    keys.emplace_back(pair.key);
  }
  if (Exists(keys, &exists).ok() && exists > 0) {
    return rocksdb::Status::OK();
  }

so the exist in for loop looks a bit redundant, in which case we need this exist (the for loop one, i.e. this PR diff)? and another question is do we want to fix this inconsistency?

Yes, we should use MGet and MSet at once instead of the loop writes.

you mean i remove the for (StringPair pair : pairs) { loop, and then just call a MSet?
something like this:

-  std::string ns_key;
-  for (StringPair pair : pairs) {
-    AppendNamespacePrefix(pair.key, &ns_key);
-    LockGuard guard(storage_->GetLockManager(), ns_key);
-    if (Exists({pair.key}, &exists).ok() && exists == 1) {
-      return rocksdb::Status::OK();
-    }
-    std::string bytes;
-    Metadata metadata(kRedisString, false);
-    metadata.expire = expire;
-    metadata.Encode(&bytes);
-    bytes.append(pair.value.data(), pair.value.size());
-    auto batch = storage_->GetWriteBatchBase();
-    WriteBatchLogData log_data(kRedisString);
-    batch->PutLogData(log_data.Encode());
-    batch->Put(metadata_cf_handle_, ns_key, bytes);
-    auto s = storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
-    if (!s.ok()) return s;
-  }
+  rocksdb::Status s = MSet(pairs, ttl);
+  if (!s.ok()) return s;
+

Yes, but need to check if any key exists before the MSET. MSETNX now is an exclusive command and it'd be better to lock those keys and then remove the exclusive flag from the command.

MSETNX now is an exclusive command and it'd be better to lock those keys and then remove the exclusive flag from the command.

I am not very familiar with the lock part, can you describe it in detail?
the code now may look like:

rocksdb::Status String::MSetNX(const std::vector<StringPair> &pairs, uint64_t ttl, bool *flag) {
  *flag = false;

  int exists = 0;
  std::vector<Slice> keys;
  keys.reserve(pairs.size());
  for (StringPair pair : pairs) {
    keys.emplace_back(pair.key);
  }
  if (Exists(keys, &exists).ok() && exists > 0) {
    return rocksdb::Status::OK();
  }

  rocksdb::Status s = MSet(pairs, ttl);
  if (!s.ok()) return s;

  *flag = true;
  return rocksdb::Status::OK();
}

do you want me to lock the keys before the Exists call (doing a MultiLockGuard guard?)? This will result in an additional AppendNamespacePrefix call since the lock need to call AppendNamespacePrefix and then in Exists it will also need to call AppendNamespacePrefix.
Note that in MSet, there is another AppendNamespacePrefix call, and Mset will do a LockGuard guard.

Yes, need to use MultiLockGuard to lock multi keys, and MSet needs to do a bit of change for supporting the non-lock calls. For calling AppendNamespacePrefix twice, I think it's fine since it is just a little CPU overhead.